web-client/src/app/pages/model/useDashboardViewModel.ts (1)

203-217: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Fetch waterfall: org/events queries gated on data?.role instead of the already-known user.role.

sportsQuery/teamsQuery/eventsQuery are only enabled once dashboardQuery resolves and exposes data.role, even though role = user.role (line 219) is available synchronously from useAuth(). This forces a sequential round trip (dashboard fetch → then org/events fetch) instead of firing them in parallel, adding latency to admin and trainee/trainer/director dashboards alike.

If the token role reliably predicts the envelope role (which shouldShowBalance below already assumes), gating on role instead of data?.role would let these fire in parallel with the dashboard call.

♻️ Proposed fix

-  const isAdmin = data?.role === 'admin'
+  const isAdmin = role === 'admin'
   const sportsQuery = useSportsList(isAdmin)
   const teamsQuery = useTeamsList(isAdmin)

   // ...
-  const isNonAdmin = !!data && data.role !== 'admin'
+  const isNonAdmin = role !== 'admin'
   const eventsQuery = useEventsList(isNonAdmin)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/app/pages/model/useDashboardViewModel.ts` around lines 203 -
217, The org/events queries in useDashboardViewModel are being gated on
dashboardQuery.data.role, which creates a fetch waterfall even though useAuth
already provides user.role synchronously. Update the enablement logic for
sportsQuery, teamsQuery, and eventsQuery to key off the already-known role (or
an equivalent derived value) instead of data?.role, while keeping the existing
admin/non-admin branching behavior intact. This will let the dashboard and
dependent queries start in parallel without changing the role-based routing in
useDashboardViewModel.

web-client/src/features/feedback/model/useFeedbackViewModel.ts (1)

290-296: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Gate useTeamsList here; useMembers needs an enabled option first
buildFeedbackCoverage() returns null for non-trainers, so useTeamsList can be skipped on those roles. useMembers is still always on, so add an enabled param there if you want to avoid that fetch too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/feedback/model/useFeedbackViewModel.ts` around lines
290 - 296, Gate the team/member fetches in useFeedbackViewModel so non-trainers
do not load unnecessary data: use the user.role check to conditionally run
useTeamsList since buildFeedbackCoverage() returns null for non-trainers, and
update useMembers to accept an enabled option so its fetch can also be skipped
when the role should not load members. Keep the change localized to
useFeedbackViewModel and the useMembers hook contract so the enabled flag
controls the query behavior.

web-client/src/mocks/fixtures/organization.ts (1)

756-758: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Hardcoded TEAM_U16 id with a name that doesn't match the referenced team.

The comment says "first team the signed-in member belongs to," but the constant is named TEAM_U16 while it points to "Football Juniors" (bbbbbbbb-0001…), which isn't described as a U16-specific squad anywhere in this file. Consider deriving this from myTeamFixtures[0]?.id (or renaming to something accurate) so it can't drift from CURRENT_MEMBER_ID/myTeamFixtures if either changes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/mocks/fixtures/organization.ts` around lines 756 - 758, The
TEAM_U16 constant is hardcoded and may drift from the actual first team for the
signed-in member. Update the fixture in organization.ts so the exported team id
is derived from myTeamFixtures[0]?.id (or rename the constant to match the
actual team it represents) and keep it aligned with CURRENT_MEMBER_ID and the
first entry in myTeamFixtures.

web-client/src/mocks/scope.test.ts (1)

1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

scopeTeamReport has no test coverage.

scope.ts exports scopeTeamReport (used to gate team-report visibility for trainer/director/member roles), but this file only imports/tests scopeBalances, scopeEvents, scopeFeedback, scopeMembers, scopeReport, scopeTransactions. Given this function guards role-based access to team reports, add a describe('scopeTeamReport', ...) block mirroring the scopeReport tests (trainer sees own-team reports, director sees own-sport's teams, member is always denied).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/mocks/scope.test.ts` around lines 1 - 22, Add test coverage
for scopeTeamReport in the scope.test suite. The current test file imports
scopeBalances, scopeEvents, scopeFeedback, scopeMembers, scopeReport, and
scopeTransactions but never exercises scopeTeamReport, so create a
describe('scopeTeamReport', ...) block alongside the existing role-based scope
tests. Mirror the scopeReport patterns using MOCK_PERSONAS and the fixtures
helpers to verify trainer access to own-team reports, director access to
own-sport teams, and that member access is always denied.

web-client/vite.config.ts (1)

21-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the rewrite regex into a testable pure function.

The path-rewrite logic (query splitting, service-segment duplication, trailing-slash trim) is non-trivial and currently only lives inline in the Vite config with no unit coverage. A regex edit (new service name, edge case) could silently break every dev API call. Since dev-proxy misbehavior is easy to miss until manual testing, a small extracted/tested helper would catch regressions early.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/vite.config.ts` around lines 21 - 34, The inline path rewrite
logic in the Vite config is too complex to leave untested, so extract it from
the rewrite callback into a pure helper that handles query splitting, service
duplication, and trailing-slash normalization. Move the regex-based
transformation from the Vite proxy’s rewrite function into a named function that
can be imported and unit tested, then keep the config callback as a thin wrapper
around that helper. Add tests for the extracted helper covering existing service
names, query strings, and trailing slash cases so future regex changes don’t
silently break API calls.

web-client/src/features/auth/currentUser.ts (1)

19-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No test coverage for the mock-persona identity path.

getCurrentUser() branches on USE_MOCKS/VITE_MOCK_PERSONA, but only the tokenUser() fallback path is exercised in useAuth.test.ts. Given this function is the single source of identity for role-gated navigation and data scoping across the app, consider adding coverage for: USE_MOCKS=true with a valid persona key, an invalid/unset VITE_MOCK_PERSONA (should default to 'member'), and USE_MOCKS=false (falls through to token).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/auth/currentUser.ts` around lines 19 - 29, Add test
coverage for the identity branching in getCurrentUser/mockPersona so the
mock-persona path is exercised, not just tokenUser(). In useAuth.test.ts, cover
USE_MOCKS=true with a valid VITE_MOCK_PERSONA key, an invalid or unset
VITE_MOCK_PERSONA defaulting to the 'member' entry in MOCK_PERSONAS, and
USE_MOCKS=false falling through to tokenUser(); use the getCurrentUser and
mockPersona symbols to locate the logic.

web-client/src/__tests__/useAuth.test.ts (1)

1-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Tests implicitly depend on USE_MOCKS being false; not isolated from env.

Both tests set keycloakMock.tokenParsed and expect useAuth() to use the token path, but getCurrentUser() now checks mockPersona() first: if (!USE_MOCKS) return null (currentUser.ts, Line 20). Since USE_MOCKS is derived from import.meta.env.VITE_USE_MOCKS at module load and neither mockSwitch nor personas is mocked here, these assertions silently rely on that env var being unset/false during the vitest run. If a root .env sets VITE_USE_MOCKS=true (loaded by Vite for any mode unless overridden by .env.test), getCurrentUser() would return a mock persona instead of the token-derived user, and both tests would fail for a reason unrelated to the code under test.

Consider explicitly mocking @/mocks/mockSwitch to force the token path, making the test deterministic regardless of environment configuration.

🧪 Suggested test isolation fix

 import { describe, expect, it, vi } from 'vitest'

+vi.mock('`@/mocks/mockSwitch`', () => ({ USE_MOCKS: false, mockOr: (_m: unknown, live: () => unknown) => live() }))
+
 const keycloakMock = {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/__tests__/useAuth.test.ts` around lines 1 - 34, The useAuth
tests are implicitly relying on the runtime value of USE_MOCKS, which makes them
environment-dependent and brittle. Update the test setup in useAuth.test.ts to
explicitly mock the mock-switch path used by currentUser/getCurrentUser so the
tokenParsed branch is always exercised, regardless of VITE_USE_MOCKS. Keep the
assertions focused on useAuth, keycloakMock, and the getCurrentUser/mockPersona
flow so the tests remain deterministic.

web-client/src/features/organization/api/queries.ts (1)

47-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider retry: false for single-item lookups.

When the mock/live lookup misses, Sport not found / Team not found is a deterministic error, not a transient one. Without retry: false, react-query's default retry behavior will retry the failed query multiple times before surfacing the error, delaying any "not found" UI state.

Also applies to: 113-127

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/organization/api/queries.ts` around lines 47 - 61,
The single-item lookup queries in useSport and the matching useTeam hook are
throwing deterministic “not found” errors, so react-query should not retry them.
Add retry: false to the useQuery options for these id-based lookups so a missing
Sport/Team fails immediately and the UI can show the not-found state without
delay.

web-client/src/features/organization/model/useTeamsViewModel.ts (1)

16-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

SportTeamsView drops id, forcing downstream code to key off name.

OrganizationPage.tsx uses sport.name for the React key, the activeOpenSport expand/collapse state, and stats.mySports (a Set of sportName). If two sports ever share the same name, this causes duplicate React keys, colliding expand/collapse state between unrelated sports, and undercounted mySports. Prefer carrying the sport id through SportTeamsView and using it as the stable identifier.

♻️ Proposed fix

 export interface SportTeamsView {
+  id: string
   name: string
   description: string
   directors: MemberRef[]
   teams: TeamView[]
 }

   const joinedSports = sports.map((sport) => ({
+    id: sport.id,
     name: sport.name,
     description: sport.description,
     directors: sport.directors,
     teams: teamsBySportId.get(sport.id) ?? [],
   }))

Also applies to: 59-64

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/organization/model/useTeamsViewModel.ts` around lines
16 - 21, SportTeamsView is missing the sport identifier, which forces downstream
consumers to rely on name as the stable key. Update the SportTeamsView interface
and the related mapping in useTeamsViewModel so the sport id is preserved
alongside name, then switch OrganizationPage.tsx and any consumers of
activeOpenSport and stats.mySports to use that id as the unique identifier
instead of sport.name.

web-client/src/features/helper/model/useReportViewModel.ts (1)

46-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Trainer with no matched team surfaces a generic "Report not allowed" error.

If team is null (trainer not yet linked to any team, or teamsQuery hasn't resolved), generateTeam is created with teamId=''; clicking generate will fail inside scopeTeamReport('', ...) and surface generateError.message as the unhelpful "Report not allowed" string in the UI, rather than indicating the real cause (no team assigned).

Consider disabling the generate action (or short-circuiting generate) when scope === 'team' && !team.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/helper/model/useReportViewModel.ts` around lines 46 -
52, The team-report generation path in useReportViewModel is creating a
generator with an empty team id when team is null, which leads to the generic
failure message instead of a clear no-team state. Update the
generateTeam/useGenerateTeamReport flow so that when scope is 'team' and team is
missing, the generate action is disabled or short-circuited before calling
scopeTeamReport, and only use generateTeam when team?.id is available.

web-client/src/features/helper/api/queries.ts (1)

96-128: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Per-report read/delete skip the scope checks used elsewhere in this file.

useReport (lines 98-111) and useDeleteReport (lines 116-128) look up/delete by reportId alone with no scopeReport/scopeTeamReport check in the mock branch, unlike useGenerateMemberReport/useGenerateTeamReport and the list hooks, which all gate access via scope. Today this is masked because reportId only ever comes from an already-scoped list row, but it's an inconsistent invariant: any code path (or future URL-driven detail view) that supplies an arbitrary reportId could read or delete a report the current mock user shouldn't have access to.

🔒 Suggested fix: reuse scope helpers for single-report access

 export function useReport(reportId: string | null) {
   return useQuery<Report>({
     queryKey: helperKeys.report(reportId ?? ''),
     enabled: !!reportId,
     queryFn: () =>
       mockOr(
         () => {
           const found = reportId ? reportById[reportId] : undefined
-          if (!found) throw new Error('Report not found')
+          if (!found) throw new Error('Report not found')
+          const allowed =
+            found.kind === 'team'
+              ? scopeTeamReport(found.team?.id ?? '', getCurrentUser())
+              : scopeReport(found.member?.id ?? '', getCurrentUser())
+          if (!allowed) throw new Error('Report not found')
           return Promise.resolve(found)
         },
         () => helperClient.get<Report>(`/reports/${reportId}`).then(r => r.data),
       ),
   })
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/helper/api/queries.ts` around lines 96 - 128, The
single-report hooks bypass the same scope guard used elsewhere, so align
`useReport` and `useDeleteReport` with the existing scoped access pattern. In
the mock branch of `queryFn` and `mutationFn`, resolve the report through
`scopeReport` or `scopeTeamReport` before reading or deleting, instead of using
`reportId` directly. Reuse the same helper logic already present in
`useGenerateMemberReport`, `useGenerateTeamReport`, and the list hooks so
`helperKeys.report`, `reportById`, and `helperClient` only operate on scoped
reports.

web-client/src/features/sport-events/model/useEventsViewModel.ts (1)

25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

missedCount is declared but never populated.

EventsView.missedCount is an optional field that buildEventsView never sets, and SportEventsPage.tsx's stat cards only read stats.upcoming/thisWeek/total. This looks like leftover scaffolding for an unfinished "missed" stat card — either wire it up or drop the field to avoid misleading future consumers who might rely on it being present.

Also applies to: 156-164

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/sport-events/model/useEventsViewModel.ts` around
lines 25 - 34, The EventsView.missedCount field is leftover scaffolding because
buildEventsView does not populate it and SportEventsPage only consumes
stats.upcoming/thisWeek/total. Either remove missedCount from EventsView and any
related typing in useEventsViewModel/buildEventsView, or wire it through
consistently by computing and assigning it where the view model is built and
updating the consuming UI if it is meant to be displayed.

web-client/src/features/sport-events/api/queries.ts (1)

86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract canManageEvent for reuse instead of duplicating the check in the UI.

SportEventsPage.tsx (line 274) reimplements this exact condition (user.role === 'admin' || detail.creator?.id === user.id) inline. Exporting this helper and importing it in the page would prevent the two authorization checks from silently drifting apart.

♻️ Proposed refactor

-function canManageEvent(user: AuthUser, event: SportEvent): boolean {
+export function canManageEvent(user: AuthUser, event: SportEvent): boolean {
   return user.role === 'admin' || event.creator?.id === user.id
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web-client/src/features/sport-events/api/queries.ts` around lines 86 - 88,
Export the canManageEvent helper from queries.ts and reuse it in SportEventsPage
instead of keeping the same authorization condition inline. Update the existing
canManageEvent function to be the single source of truth for the
admin-or-creator check, then import and call it from the page where the inline
user.role === 'admin' || detail.creator?.id === user.id logic currently lives.
This keeps the permission rule centralized and prevents the two checks from
diverging.

❌ Autofix failed (check again to retry)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes